hmm my version and yours of rustfmt are not in sync

hmm my version and yours of rustfmt are not in sync

The struggle is real. This is how we attempted to work around this:
https://github.com/oxidecomputer/dropshot/blob/master/.vscode/settings.json

We pinned down a specific version because we found the spurious changes to be annoying:
https://github.com/oxidecomputer/dropshot/blob/master/.github/workflows/rust.yml#L20

Cool! Thanks for doing this.

What do you think about using the Default trait for ConfigDropshot instead of an Option here? I haven't used Default much myself, but I think it would be more clearly express what's going on. When I saw this was an Option, I expected that None would mean "no limit" not "default limit".

Yes, I think it would be great to add the other two fields here (definitely not a blocker for this PR).

Can’t implement default on a type like usize want me to make a new type for it

Did that :)

I reckon we could provide a manual implementation of Default for ConfigDropshot which would return a copy of the struct with Some(1024) (or whatever) filled in as the default value for the new property?

Then you could...

let cfg = ConfigDropshot {
    bind_address: "127.0.0.1:0".parse().unwrap(),
    ..Default::default()
};

... and you'd be proofed against future additional fields.

Yeah, I was talking about implementing Default for ConfigDropshot, not a new type. (I don't have a feeling either way about using a new type here.) This would make it easy for us to provide defaults for other fields in the future. (I wonder if it would make it harder for us to force people to specify fields for which it doesn't make sense to have a default?)

Either way is okay right now but it looks like both style and the test suite aren't passing. Are there tests for the new change? I don't see them but maybe it's exercised by the tests that are failing?

Also those tests fail for me on master so I wonder if it's something with my setup..? Will look into it.

Okay re-did this so that it is Default on DropshotConfig.

Also sorry about the fmt changing....
And I am unsure why the tests fail for me on master as well, going to try to dive in

Also I had to run the tests with TEST_OVERWRITE locally and sorry those are reflected in the diff. If I should not commit those files I can easily remove that commit.

also I am sure it's there for a reason but what do you think about removing the "version" line from the rustfmt file? that's how I was able to fmt locally, but I didn't commit that change

also I am sure it's there for a reason but what do you think about removing the "version" line from the rustfmt file? that's how I was able to fmt locally, but I didn't commit that change

That was how we ensured that we didn't wrestle over formatting. At least at the time we found that recent changes in rustfmt impacted our code. I think this is what we used previously:

rustup install nightly-2020-06-02
rustup run nightly-2020-06-02 rustfmt <file>

If you're using vscode, it should "do the right thing"

I think it would be very reasonable to update the version number in the rustfml.toml to reflect a more recent version; you'll want to update .vscode/settings.json and .github/workflows/rust.yml as well.

.. and to be clear: this all is pretty jank and we wish that there was some more reasonable way to ensure a consistent formatting of code. For example, it would be cool if the Cargo.toml could depend on a specific rustfmt version and use it. The version in rustfmt.toml which acts only to induce failures ... sucks.

Thanks for doing this and sticking with it! I like the Default impl and the expected test output changes look like the usual compiler churn (unfortunately), so that's expected too. Sorry it took so long for me to circle back on this.